[Experimental Bot, please feedback here]

No, this PR likely does not fully meet the NuttX requirements based on the provided information. Here's why:

• Insufficient Summary: While it states what was changed (added CMake scripts for QuickJS), it lacks crucial details:

  • Why is this change necessary? Is it to improve build times, simplify the build process, or address a specific issue?
  • How does it work? Does it replace existing build mechanisms? How does it integrate with the NuttX build system? What files are involved?

• Incomplete Impact Assessment: "nop" is not acceptable. While the impact might be minimal, the submitter must explicitly consider and address each point:

  • Does this affect users? Will they need to change how they build or use QuickJS?
  • Does this affect the build process itself beyond just adding the CMake option?
  • Does it affect any hardware or specific architectures?
  • Does it require documentation updates? Even if the answer is no, it should be stated explicitly.
  • Security, compatibility, etc., all need explicit "NO" or a detailed explanation if "YES."

• Inadequate Testing: The testing description is very superficial. It doesn't show what was tested. Just building and enabling the configuration options is not sufficient. There needs to be evidence that QuickJS actually works within NuttX after this change. The logs are empty, providing zero evidence of functional testing. Specific test cases and their results before and after the change are required. Information on the build host is also missing.

To meet the requirements, the PR needs the following:

• Expanded Summary: Explain the rationale for the change and provide more technical details on how the CMake scripts are integrated.

• Detailed Impact Assessment: Explicitly address each impact category, even if the answer is "NO."

• Thorough Testing Information:

  • Specify the build host operating system, CPU architecture, and compiler version.
  • Provide specific test cases executed, including expected outcomes.
  • Include actual "before" and "after" logs demonstrating the functionality of QuickJS both before the change (presumably using the old build system) and after using the new CMake approach. These logs should show the QuickJS interpreter being invoked and producing expected output.

Without these improvements, the PR is likely to be rejected or require significant revisions.

Sorry I have to cancel the CI Check, there's a problem with CI Test:

• drivers/pipes: return after short write if buffer is full nuttx#14667 (comment)

I have restarted the CI task, is that okay? :-P

@cederom Sure but sim-01 will run CI Test very slowly, and after 6 hours, GitHub will kill it anyway due to timeout. I was chatting about it here: apache/nuttx#14680

And we're trying to fix it now: #2829

ACK! Sorry, task cancelled :-)

@cederom The (workaround) fix has just been merged: #2829

Hopefully all jobs in NuttX Repo and NuttX Apps won't hang any more :-)

@cederom Ack sorry my mind is slow - If you need to restart any of the jobs I cancelled earlier, please click "Restart All Jobs", not "Restart Failed Jobs". This will rerun Fetch-Source to pick up the latest fixes from NuttX Apps. Thanks :-)

Please restart yourself @lupyuen when ready :-) Thank you for the hint on restart-all to re fetch all updates!! :-)

@WenLY1 I have problem testing this PR locally.. my first cmake steps in NuttX.. could you please advise? :-)

(venv3.11embedded) uname -a
Linux octagon 5.15.0 FreeBSD 13.3-RELEASE-p7 GENERIC x86_64 GNU/Linux

Looks like cmake tries to create /include directory on your branch why?

(venv3.11embedded) rm -rf build/

(venv3.11embedded) git checkout WenLY1/quickjs_upstream
Updating files: 100% (27852/27852), done.
Note: switching to 'WenLY1/quickjs_upstream'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 7191a587c2 update quickjs patch for cmake compilation


(venv3.11embedded) git branch
* (HEAD detached at WenLY1/quickjs_upstream)
  br_wjy_kernel_sotest_devminor
  master

(venv3.11embedded) cmake -B build -DBOARD_CONFIG=sim:nsh
CMake Warning (dev) in CMakeLists.txt:
  No project() command is present.  The top-level CMakeLists.txt file must
  contain a literal, direct call to the project() command.  Add a line of
  code such as

    project(ProjectName)

  near the top of the file, but after cmake_minimum_required().

  CMake is pretending there is a "project(Project)" command on the first
  line.
This warning is for project developers.  Use -Wno-dev to suppress it.

-- The C compiler identification is GNU 12.2.0
-- The CXX compiler identification is GNU 12.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:41 (file):
  file problem creating directory: /include


CMake Error at audioutils/CMakeLists.txt:21 (nuttx_add_subdirectory):
  Unknown CMake command "nuttx_add_subdirectory".


CMake Warning (dev) in CMakeLists.txt:
  No cmake_minimum_required command is present.  A line of code such as

    cmake_minimum_required(VERSION 3.25)

  should be added at the top of the file.  The version specified may be lower
  if you wish to support older CMake versions for this project.  For more
  information run "cmake --help-policy CMP0000".
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Configuring incomplete, errors occurred!
See also "/XXX/nuttx/build/CMakeFiles/CMakeOutput.log".
(venv3.11embedded) rm -rf build/

There is no problem on nuttx master all works fine:

(venv3.11embedded) git checkout master
Updating files: 100% (27852/27852), done.
Previous HEAD position was 7191a587c2 update quickjs patch for cmake compilation
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

(venv3.11embedded) cmake -B build -DBOARD_CONFIG=sim:nsh
-- Initializing NuttX
  Select HOST_LINUX=y
  Select HOST_X86_64=y
--   CMake   3.25.1
--   Board:  sim
--   Config: nsh
--   Appdir: /XXX/nuttx-apps
-- The C compiler identification is GNU 12.2.0
-- The CXX compiler identification is GNU 12.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /usr/bin/cc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /XXX/nuttx/build

@cederom Thanks for your testing work. I tested the command cmake -B build -DBOARD_CONFIG=sim:nsh under the directory nuttx, it seems that you are testing this command under the directory nuttx-apps?

No, under nuttx/ :-) And in the same location after checkout to master build works fine..

@cederom Sorry, the testing steps is not clear. This PR is for nuttx-apps, could you apply this PR under directory nuttx-apps and then test the command cmake -B build -DBOARD_CONFIG=sim:nsh under the directory nuttx?

@WenLY1 For easier testing: Maybe we can create a new defconfig in NuttX Repo that will build sim:nsh with QuickJS? Please keep the defconfig in a Draft PR, so it won't be accidentally merged. Thanks!

@lupyuen Sure. The defconfig is given in apache/nuttx#14693

Thanks @WenLY1! Sorry I have a compile error on macOS, did I miss something? (I'll retest on Ubuntu)

$ git clone https://github.com/WenLY1/nuttx --branch qjs_defconfig
$ git clone https://github.com/WenLY1/nuttx-apps apps --branch quickjs_upstream
$ cd nuttx
$ cmake -B build -DBOARD_CONFIG=sim:nsh
$ cmake --build build
/tmp/241108/nuttx/fs/vfs/fs_pseudofile.c:99:3: error: incompatible function pointer types initializing 'ssize_t (*)(struct file *, const struct uio *)' (aka 'long (*)(struct file *, const struct uio *)') with an expression of type 'int (struct inode *)' [-Wincompatible-function-pointer-types]
   99 |   pseudofile_unlink,   /* unlink */
      |   ^~~~~~~~~~~~~~~~~

https://gist.github.com/lupyuen/784f0c9bd84e5c009e264664017723b2

Update: Yep it works OK on Ubuntu. Let's ignore macOS thanks!